Fix "weave ps" #2418

brb · 2016-07-04T14:04:00Z

This PR fixes the problem described in #2388. In addition, the PR optimizes the GetBridgeNetDevs and containerAddrs functions.

Keep in mind, that the fix is specific for this case, because we move all the code depending on a non-host netns to the bottom of containerAddrs instead of making WithNetNS safe.

To address the safety problem, I've filed #2419 .

Fixes #2388.

common/utils.go

+func GetBridgeNetDev(bridgeName string) (NetDev, error) {
+	var netdev NetDev
+
+	err := weavenet.WithNetNSLinkByPid(1, bridgeName, func(link netlink.Link) error {


prog/weaveutil/addrs.go

+
+	containers := make(map[string]*docker.Container)
+	for _, cid := range containerIDs {
+		if containers[cid], err = client.InspectContainer(cid); err != nil {


prog/weaveutil/addrs.go

-		return common.GetBridgeNetDev(bridgeName)
-	}
-
-	container, err := c.InspectContainer(containerID)


- Optimize. Use LinkByName instead of iterating over list of available links. Linux does not support interfaces with the same names. - Remove unnecessary indirection in utility functions which return common.NetDev.

Make sure that any network namespace changes happens at the end of the cmd invocation. Otherwise, we are risking to end up in a situation in which OS scheduling threads are in a "wrong" namespace. See WithNetNS for more details.

Please see #2388 (comment) for more details.

- Do not re-enter the root netns when querying weave bridge. - Enter the root netns when constructing the predicate.

common/utils.go

+	indexes := make(map[int]struct{})
+
+	// Scan devices in root namespace to find those attached to weave bridge
+	err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName,


bboreham · 2016-07-11T12:43:50Z

LGTM assuming my questions for clarification come out the way I think.

The #2418 has changed a behaviour of "weave ps" which outputs "Link not found" when weave is not running. The previous behaviour was to output an empty line.

The #2418 has changed a behaviour of "weave ps" which outputs "Link not found" in the case of weave not running. The previous behaviour was to output an empty line.

bboreham self-assigned this Jul 5, 2016

bboreham reviewed Jul 5, 2016
View reviewed changes

bboreham assigned brb and unassigned bboreham Jul 5, 2016

bboreham reviewed Jul 5, 2016
View reviewed changes

prog/weaveutil/addrs.go

containers := make(map[string]*docker.Container)

for _, cid := range containerIDs {

if containers[cid], err = client.InspectContainer(cid); err != nil {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

brb force-pushed the issues/2388-weave-ps branch from 84ab810 to c98ed0b Compare July 5, 2016 13:56

brb reviewed Jul 5, 2016
View reviewed changes

prog/weaveutil/addrs.go

return common.GetBridgeNetDev(bridgeName)

}

container, err := c.InspectContainer(containerID)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

brb force-pushed the issues/2388-weave-ps branch from c98ed0b to fa920c6 Compare July 5, 2016 14:10

brb added 4 commits July 5, 2016 15:12

Refactor common.GetBridgeNetDev

05baa56

- Optimize. Use LinkByName instead of iterating over list of available links. Linux does not support interfaces with the same names. - Remove unnecessary indirection in utility functions which return common.NetDev.

Fix "weave ps"

7f5c37e

Make sure that any network namespace changes happens at the end of the cmd invocation. Otherwise, we are risking to end up in a situation in which OS scheduling threads are in a "wrong" namespace. See WithNetNS for more details.

Denote WithNetNS* functions with unsafe

89221f1

Please see #2388 (comment) for more details.

Address PR comments

0861d05

- Do not re-enter the root netns when querying weave bridge. - Enter the root netns when constructing the predicate.

brb force-pushed the issues/2388-weave-ps branch from fa920c6 to 0861d05 Compare July 5, 2016 14:12

brb assigned bboreham and unassigned brb Jul 5, 2016

bboreham reviewed Jul 11, 2016
View reviewed changes

common/utils.go

indexes := make(map[int]struct{})

// Scan devices in root namespace to find those attached to weave bridge

err := weavenet.WithNetNSLinkByPidUnsafe(1, bridgeName,

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

bboreham assigned brb and unassigned bboreham Jul 11, 2016

brb assigned bboreham and unassigned brb Jul 11, 2016

bboreham added this to the 1.6.1 milestone Jul 12, 2016

bboreham merged commit c1483db into 1.6 Jul 12, 2016

brb added a commit that referenced this pull request Jul 14, 2016

Fix "weave ps" output when weave is not running

26fe5a2

The #2418 has changed a behaviour of "weave ps" which outputs "Link not found" when weave is not running. The previous behaviour was to output an empty line.

This was referenced Jul 14, 2016

Fix "weave ps" output when weave is not running #2444

Closed

Fix "weave ps" output when weave is not running #2445

Merged

brb added a commit that referenced this pull request Jul 14, 2016

Fix "weave ps" output when weave is not running

fd0a7ef

The #2418 has changed a behaviour of "weave ps" which outputs "Link not found" in the case of weave not running. The previous behaviour was to output an empty line.

bboreham deleted the issues/2388-weave-ps branch November 9, 2016 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "weave ps" #2418

Fix "weave ps" #2418

brb commented Jul 4, 2016 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Jul 11, 2016

Fix "weave ps" #2418

Fix "weave ps" #2418

Conversation

brb commented Jul 4, 2016 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Jul 11, 2016

brb commented Jul 4, 2016 •

edited

Loading